Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bring QUIC Back #291

Merged
merged 4 commits into from
Aug 21, 2024
Merged

Bring QUIC Back #291

merged 4 commits into from
Aug 21, 2024

Conversation

pi-314159
Copy link
Member

No description provided.

@pi-314159
Copy link
Member Author

@bhess @baentsch Could you take a look at this?

@pi-314159 @Raytonne @lxwzy
Add nginx with QUIC Dockerfile
5f56576 
Co-authored-by: Raytonne <[email protected]>
Co-authored-by: lxwzy <[email protected]>
Co-authored-by: pi-314159 <[email protected]>
Signed-off-by: pi-314159 <[email protected]>
baentsch
baentsch requested changes Aug 13, 2024
View reviewed changes
nginx/README-QUIC.md Outdated
docker build -f Dockerfile-QUIC .
```

After building, remember the SHA256 hash of the image from the last line of the output.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks complicated: Why not built to a name (and reference that below)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

nginx/README-QUIC.md Outdated
- **Without Port Forwarding:**

```bash
docker run -d SHA256_OF_THE_IMAGE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use name

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

nginx/README-QUIC.md Outdated
- **With Port Forwarding:**

```bash
docker run -d -p 80:80 -p 443:443 -p 443:443/udp SHA256_OF_THE_IMAGE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use name

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

nginx/README-QUIC.md Outdated

Replace `CONTAINER_ID` with the ID obtained from the previous step.

Inside the container, nginx configuration files are located in `/etc/nginx`, and the nginx executable is at `/usr/sbin/nginx`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm -- this leaves users pretty much at their own devices... Other demos have USAGE.md files to help people getting going...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

partially fixed by providing an example server configuration...
I think it's straightforward for users who have used nginx before...

@baentsch
Copy link
Member

baentsch commented Aug 13, 2024 •
edited
Loading

Thanks for the new integration @pi-314159 ! Do I get it right that this drops a QUIC-enabled client- and server- demo adding back only a server side?

Edit/add: I guess what I'm asking for is a (new) USAGE-QUIC.md file that tells people simply running the binary dockerimage how they can test-drive (and configure for all supported alg combinations) the image? Or don't you want to make this possible (as also the docker build-and-push CI commands are missing)?

@bhess
Copy link
Member

bhess commented Aug 13, 2024

Thank you for adding this QUIC-demo @pi-314159, this is really useful also as addition to the test server. Also tagging @ajbozarth who is looking at updating the oqs-demos.

Two remarks:

  • Does USAGE.md warrant an update? (e.g. how to connect to nginx-quic with a client)
  • Since this removes the demo in the quic-folder, does it provide the same functionality?

@baentsch
Copy link
Member

this is really useful also as addition to the test server

Are you going to deploy this on the IBM test server instance @bhess or are you expecting someone else to write code, config and documentation?

@bhess
Copy link
Member

bhess commented Aug 13, 2024

Are you going to deploy this on the IBM test server instance @bhess

This was my thinking.

@pi-314159
Copy link
Member Author

@baentsch @bhess

Does USAGE.md warrant an update? (e.g. how to connect to nginx-quic with a client)

I'll add a QUIC enabled curl Dockerfile in the curl folder after this PR gets merged.

Since this removes the demo in the quic-folder, does it provide the same functionality?

Previously, we were using quictls; BoringSSL lacked support for some algorithms. However, I plan to add hybrid algorithm implementations to BoringSSL this week.

Edit/add: I guess what I'm asking for is a (new) USAGE-QUIC.md file that tells people simply running the binary dockerimage how they can test-drive (and configure for all supported alg combinations) the image? Or don't you want to make this possible (as also the docker build-and-push CI commands are missing)?

One reason I didn't add such feature is that BoringSSL's bssl don't support creating X509 certificates. This means I'd also need to build the oqs-provider to generate a test certificate, which adds extra complexity. Instead, I can provide an nginx-quic-example.conf file to help with this.

@pi-314159
update
809320b 
Signed-off-by: PI <[email protected]>
@dstebila dstebila mentioned this pull request Aug 14, 2024
Closed
@pi-314159 pi-314159 requested a review from baentsch August 19, 2024 05:19
baentsch
baentsch requested changes Aug 19, 2024
View reviewed changes
Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a QUIC enabled curl Dockerfile in the curl folder after this PR gets merged.

First removing the (old) QUIC demo completely before adding back functionality that has been removed doesn't sound good. If you'd like to proceed that way (this PR first, another PR later), then please remove the "quic removal" logic from this PR and add it to the second PR so we know the for sure we have a new demo replacing the old one functionally.

@pi-314159
Copy link
Member Author

@baentsch and @bhess added curl

@pi-314159 pi-314159 requested a review from baentsch August 19, 2024 09:21
@pi-314159 pi-314159 linked an issue Aug 19, 2024 that may be closed by this pull request
Add QUIC support #261
Closed
@pi-314159 pi-314159 self-assigned this Aug 19, 2024
@baentsch
Copy link
Member

@pi-314159 Thanks for the curl addition! I now pushed your code into the OQS repo. 2 reasons: 1) See CI run (hopefully not fail) and 2) ask you to check whether you now have the permissions to keep working on that branch (open-quantum-safe/tsc@e73b3d2 should have given you those rights). If that'd be OK, it'd be great if your new contributions could be done straight within OQS/oqs-demos such as for your jobs to also use the docker credentials upon push/PR. On that topic: Do you intend to add code to build and push the docker images? If so, also some code to test the resultant images? Examples for build-and-push in https://github.com/open-quantum-safe/oqs-demos/blob/main/.github/workflows/linux.yml and for testing in

oqs-demos/.circleci/config.yml

Lines 87 to 93 in e810c8a

- run:
name: Test nginx and curl generic
command: |
docker network create nginx-test &&
docker run --network nginx-test --detach --rm --name oqs-nginx oqs-nginx-img &&
sleep 2 &&
docker run --network nginx-test oqs-curl-generic curl -k https://oqs-nginx:4433
(the latter of course just as reference; actual testing should be in GH CI as we want to move off CCI).

@pi-314159
Copy link
Member Author

@baentsch I have the permission to merge.

I'm not very familiar with GitHub CI at the moment, so I'll take care of adding the code to build and push the Docker images a bit later. If that works for you, I'll go ahead and merge it now.

Also, please check the boringssl PR

@baentsch
Copy link
Member

I'll take care of adding the code to build and push the Docker images a bit later.

Created #294 to do this "for good" for all docker image creation. So please add your code only to GH workflow files.

I'll go ahead and merge it now.

I'd have preferred to check everything's working locally (say in a docker network), but I assume you did (?) and don't find the time myself right now, so go ahead.

@baentsch baentsch mentioned this pull request Aug 21, 2024
Closed
@pi-314159
Copy link
Member Author

I assume you did (?) and don't find the time myself right now, so go ahead.

Yes I've tested these Dockerfiles on my machine. Please test them when you have time!

@pi-314159 pi-314159 changed the title Add nginx with QUIC Dockerfile Bring cURL Back Aug 21, 2024
@pi-314159 pi-314159 merged commit 3725dba into open-quantum-safe:main Aug 21, 2024
2 of 4 checks passed
@pi-314159 pi-314159 deleted the nginx-quic-20240812 branch August 21, 2024 08:04
@pi-314159 pi-314159 changed the title Bring cURL Back Bring QUIC Back Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baentsch baentsch Awaiting requested review from baentsch

Assignees

@pi-314159 pi-314159

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add QUIC support
3 participants
@pi-314159 @baentsch @bhess